-
Notifications
You must be signed in to change notification settings - Fork 157
Feature - Environment Variable "Updated" field and pending update endpoint #4006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I'm also wondering if this may need some changes around the state of the last deployment. For example, if it fails for whatever reason before the changes are actually applied by a build, the pending changes would show as none, but not been applied. Would it be worth selecting the last deployment based on Edit: Now checks deployment status ✔️ |
Co-authored-by: Ben Jackson <[email protected]>
Co-authored-by: Ben Jackson <[email protected]>
Co-authored-by: Ben Jackson <[email protected]>
Overview
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all works for me. I haven't fully gone over the SQL query as it is quite a monster.
I ran through a few different scenarios with ordering variable additions etc, and I think there is only one thing that I might change, and that would be that the completed timestamp of the build is probably the wrong one to use, and the created timestamp with the completed status should be used.
The reason being, when the build is created, the data sent to the build contains whatever was in the API at that time. If a variable is added AFTER that build is created, it is considered a pending change still, as the last build wouldn't have known about it.
Co-authored-by: Ben Jackson <[email protected]>
Co-authored-by: Ben Jackson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this is ✔️ from me
Co-authored-by: Ben Jackson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, this will be super useful to extend in #3990 to show pending route changes too.
|
Change of plan, lets change the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, enum makes a lot of sense here when we consider future pending changes. We can adjust the details section overtime, or even add new fields as necessary as other pending change types appear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 Great!
|
Minimal but extensible addition 🎉 |
This PR introduces two things.
First, the API exposes a new set of data on the environments - namely, whether they have any pendingChanges.
These are simple,
{type:,details}messages that indicate that there are pending changes to an environment (with the implication being that a redeploy is necessary).Secondly - it introduces an "updated" field to the environment variables table that keeps track of when the last time an env var was update. This is used to check, against the last deployment, whether or not a redeploy is necessary.
General Checklist
Database Migrations
Description
Explain the details for making this change. What existing problem does the pull request solve?
Closing issues
Put
closes #XXXXin your comment to auto-close the issue that your PR fixes (if such).